Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[i886] - update logic to fix collection thumbnail bug #2381

Merged
merged 2 commits into from
Nov 14, 2024
Merged

[i886] - update logic to fix collection thumbnail bug #2381

merged 2 commits into from
Nov 14, 2024

Conversation

ShanaLMoore
Copy link
Collaborator

The default work thumbnail was displaying even though the default collection thumbnail was set.

Issue:

When No Collection Branding is Set

When Default Collection Thumbnail is Set

  • Collections should render the default thumbnail

Screenshot 2024-11-13 at 13-49-38 Show Appearance __ Hyku

Screenshot 2024-11-13 at 13-50-18 Collections

When Default Collection Thumbnail is NOT set

  • Collections should render Hyrax's default thumbnail

Screenshot 2024-11-13 at 13-50-54 Show Appearance __ Hyku

Screenshot 2024-11-13 at 13-51-05 Collections

When Collection Branding is Set

When Default Collection Thumbnail is Set

  • Collections should render the Branding thumbnail

Screenshot 2024-11-13 at 13-50-37 Edit User Collection es __ Hyku

Screenshot 2024-11-13 at 15-40-53 Collections

@ShanaLMoore ShanaLMoore added the patch-ver for release notes label Nov 13, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 14, 2024 •
edited
Loading

Test Results

    3 files  ±0      3 suites  ±0   17m 55s ⏱️ +13s
2 042 tests +2  1 991 ✅ +2  50 💤 ±0  1 ❌ ±0 
2 069 runs  +2  2 016 ✅ +2  52 💤 ±0  1 ❌ ±0 

For more details on these failures, see this check.

Results for commit d0a437f. ± Comparison against base commit 8cf3a6f.

This pull request removes 42 and adds 44 tests. Note that renamed tests count towards both. 
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor Etd permissions is expected not to be able to destroy 6df95f38-2284-48dc-90da-74f8e2d534e7
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor Etd permissions is expected not to be able to edit f5234842-fc4a-4cc0-b259-fa5c45f4a0d0
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor Etd permissions is expected not to be able to read cf13596d-3498-4852-b704-7008dd755a41
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor Etd permissions is expected not to be able to update cdc04d97-794b-47cd-8f67-e2d1262a4206
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor FileSet permissions is expected not to be able to destroy 0ee06ccb-f816-41ac-8eb7-c330279d37a5
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor FileSet permissions is expected not to be able to edit a1b32a54-2dc6-4205-836a-8300338b178e
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor FileSet permissions is expected not to be able to read eb9e8faf-1b6c-4789-8fee-10c337dec923
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor FileSet permissions is expected not to be able to update 8fec72d5-e094-404f-8a10-9843653b508a
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor GenericWork permissions is expected not to be able to destroy a28e5006-8694-4f47-b5bb-84afd1597eba
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor GenericWork permissions is expected not to be able to edit 716c1da2-cb5e-4345-a87a-beac86b5f8ca
…

spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor Etd permissions is expected not to be able to destroy 50475075-16f1-40ac-a825-08a45dfddfc6
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor Etd permissions is expected not to be able to edit da34dab5-7c33-4eb0-810c-2ac4a4dfcdf7
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor Etd permissions is expected not to be able to read ae6b28a3-0924-4a06-817d-7baaae225dab
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor Etd permissions is expected not to be able to update 8ca3d0e7-03dc-4b79-a6d0-94f426601b7f
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor FileSet permissions is expected not to be able to destroy 7a8f5434-8921-4a4b-b585-5ace7a05442a
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor FileSet permissions is expected not to be able to edit a999dfc3-20f9-4100-92e8-fb58dd6191e9
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor FileSet permissions is expected not to be able to read 5134dcfa-f879-41b4-b73a-07d900cd135a
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor FileSet permissions is expected not to be able to update e2f090ec-d045-44f7-9454-67e7ab3525ba
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor GenericWork permissions is expected not to be able to destroy 702d95fc-b9e7-4782-9d59-e337de1ffd24
spec.abilities.work_ability_spec ‑ Hyrax::Ability::WorkAbility when work depositor GenericWork permissions is expected not to be able to edit 8d1658f1-da42-4d9b-8688-07991643a998
…

♻️ This comment has been updated with latest results.

@ShanaLMoore ShanaLMoore added reindex required patch-ver for release notes and removed patch-ver for release notes labels Nov 14, 2024
@ShanaLMoore ShanaLMoore marked this pull request as draft November 14, 2024 16:24
laritakr
laritakr previously approved these changes Nov 14, 2024
View reviewed changes
@ShanaLMoore
[i886] - Fix collection thumbnail fallback logic in Hyrax::ThumbnailP…
7e77eb6 
…athServiceDecorator

The default work thumbnail was displaying even though the default collection thumbnail was set.

Issue:
- scientist-softserv/adventist_knapsack#886

- Updated #call method to use default_collection_image for collection resources when thumbnail_id is blank.
- Ensures collections without specific thumbnails use the site-wide default collection image instead of the default work image.
- Improved fallback logic to distinguish between collections and works, avoiding issues with collections displaying the work default image.
@ShanaLMoore ShanaLMoore force-pushed the i886-default-collection-thumbnail-bug branch from 771c95f to 7e77eb6 Compare November 14, 2024 16:54
@ShanaLMoore
Copy link
Collaborator Author

The failing specs gets resolved by updating Hyrax. ref: #2382

@ShanaLMoore
✅ add spec for default_collection_image in `ThumbnailPathServiceDec…
d0a437f 
…orator`

- Added a test for `default_collection_image` to verify it returns the site-specific default collection image when available.
- Stubbed `Site.instance` to ensure the spec accurately tests behavior when a site default image is present or absent.
@ShanaLMoore ShanaLMoore marked this pull request as ready for review November 14, 2024 19:52
@ShanaLMoore
Copy link
Collaborator Author

ShanaLMoore commented Nov 14, 2024 •
edited
Loading

NOTES

This was tested in adventist knapsack as well as hyku demo, and it appears to be working.

As an aside, locally I was getting this error when trying to set a thumbnail in Adventist knapsack, but the same error happened when using main (of both adventist knapsack and hyku). This error does not occur in staging or prod so I'm not sure what this is about.

To reproduce, spin up main Adventist Knapsack. In the submodule check out this branch. Spin the project up, create a collection. Click the branding tab. Upload and save a thumbnail file. 🤔

image

@ShanaLMoore
Copy link
Collaborator Author

the spec and lint failure are coming from main so I'm going to go ahead and merge this

@ShanaLMoore ShanaLMoore merged commit dd65a0e into main Nov 14, 2024
7 of 9 checks passed
@ShanaLMoore ShanaLMoore deleted the i886-default-collection-thumbnail-bug branch November 14, 2024 22:34
@ShanaLMoore ShanaLMoore mentioned this pull request Nov 14, 2024
Merged
3 tasks
ShanaLMoore added a commit to scientist-softserv/adventist_knapsack that referenced this pull request Nov 15, 2024
@ShanaLMoore
Update hyku to pull in default collection thumbnail fix (#891)
cd10a71 
Issue:
- #886

ref: 
- HYKU: samvera/hyku#2381

# When No Collection Branding is Set

## When Default Collection Thumbnail is Set
- [ ] Collections should render the default thumbnail

![Screenshot 2024-11-13 at 13-49-38 Show Appearance __
Hyku](https://github.com/user-attachments/assets/e569edc4-4ae7-4117-bd2c-86259054aab1)

![Screenshot 2024-11-13 at 13-50-18
Collections](https://github.com/user-attachments/assets/58f5a5eb-e693-4afc-aa43-a02d21e47b71)


## When Default Collection Thumbnail is NOT set
- [ ] Collections should render Hyrax's default thumbnail

![Screenshot 2024-11-13 at 13-50-54 Show Appearance __
Hyku](https://github.com/user-attachments/assets/29a74933-c7f1-40b5-b20d-a46fe28bd1da)


![Screenshot 2024-11-13 at 13-51-05
Collections](https://github.com/user-attachments/assets/ce945c3b-41c8-4a4d-b22b-6e5fd79dddce)



# When Collection Branding is Set

## When Default Collection Thumbnail is Set
- [ ] Collections should render the Branding thumbnail

![Screenshot 2024-11-13 at 13-50-37 Edit User Collection es __
Hyku](https://github.com/user-attachments/assets/78569507-1f36-4c99-8879-fd391b4929f6)



![Screenshot 2024-11-13 at 15-40-53
Collections](https://github.com/user-attachments/assets/fe0b7100-0404-43b1-a074-d26fa4f3fea9)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laritakr laritakr laritakr approved these changes

Assignees
No one assigned
Labels
patch-ver for release notes reindex required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ShanaLMoore @laritakr